ably/transport/defaults.py (2)

47-57: Refine hostname derivation logic
These conditionals cover typical cases (FQDN, localhost, nonprod, main), but consider additional validations for custom endpoints or IPv6 addresses (beyond a simple :: check).

---

60-69: Evaluate fallback logic for different endpoint patterns
The fallback domain logic is straightforward for nonprod: and main, but consider whether the same approach is needed for other future or custom endpoint patterns.

Also applies to: 72-76

test/ably/rest/restinit_test.py (2)

79-79: Fix Yoda conditions in assertions.

The assertions use Yoda conditions which are not recommended in Python. Let's make them more readable.

-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

-        assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host

Also applies to: 84-84

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

185-185: Fix line length and Yoda conditions in assertions.

The lines exceed the maximum line length of 115 characters and use Yoda conditions.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host)
+        expected_url = 'https://main.realtime.ably.net'
+        actual_url = '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host)
+        assert actual_url == expected_url

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host)
+        expected_url = 'http://main.realtime.ably.net'
+        actual_url = '{0}://{1}'.format(ably.http.preferred_scheme, ably.http.preferred_host)
+        assert actual_url == expected_url

Also applies to: 191-191

🧰 Tools

🪛 Ruff (0.8.2)

185-185: Yoda condition detected

(SIM300)

🪛 GitHub Actions: Linting check

[error] 185-185: E501 line too long (123 > 115 characters)

ably/types/options.py (2)

12-13: Fix line length in constructor parameters.

The line exceeds the maximum line length of 115 characters.

-                 tls_port=0, use_binary_protocol=True, queue_messages=False, recover=False, endpoint=None,
-                 environment=None, http_open_timeout=None, http_request_timeout=None, realtime_request_timeout=None,
+                 tls_port=0, use_binary_protocol=True, queue_messages=False,
+                 recover=False, endpoint=None, environment=None,
+                 http_open_timeout=None, http_request_timeout=None,
+                 realtime_request_timeout=None,

🧰 Tools

🪛 GitHub Actions: Linting check

[error] 13-13: E501 line too long (116 > 115 characters)

---

286-290: Consider adding a docstring for the fallback hosts logic.

The fallback hosts logic has been updated to use endpoint-based configuration, but the logic could benefit from documentation explaining the behavior.

         fallback_hosts = self.fallback_hosts
         if fallback_hosts is None:
+            # If rest_host is explicitly set, don't use fallback hosts
+            # Otherwise, get fallback hosts based on the endpoint
             if self.rest_host:
                 fallback_hosts = []
             else:
                 fallback_hosts = Defaults.get_fallback_hosts(self.endpoint)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/ably/rest/restinit_test.py (1)

184-221: 🛠️ Refactor suggestion

Fix Yoda conditions and update environment to endpoint.

Update the test to use endpoint instead of environment and fix Yoda conditions.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'https://main.realtime.ably.net'

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'http://main.realtime.ably.net'

-        ably = AblyRest(token='token', environment='custom')
+        ably = AblyRest(token='token', endpoint='custom')

🧰 Tools

🪛 Ruff (0.8.2)

185-187: Yoda condition detected

(SIM300)

---

194-196: Yoda condition detected

(SIM300)

---

207-207: Yoda condition detected

Rewrite as excinfo.value.status_code == 401

(SIM300)

---

208-208: Yoda condition detected

Rewrite as excinfo.value.code == 40103

(SIM300)

---

209-209: Yoda condition detected

(SIM300)

---

215-218: Use contextlib.suppress(AblyException) instead of try-except-pass

Replace with contextlib.suppress(AblyException)

(SIM105)

ably/transport/defaults.py (1)

60-77: Consider adding docstring to document the fallback host pattern.

The method correctly implements the fallback host generation logic but would benefit from documentation explaining the pattern used for both production and non-production environments.

 @staticmethod
 def get_fallback_hosts(endpoint="main"):
+    """Generate a list of fallback hosts based on the endpoint.
+    
+    Args:
+        endpoint (str, optional): The endpoint to generate fallback hosts for. Defaults to "main".
+            Can be a routing policy name (e.g., "main"), a non-production routing policy name
+            (e.g., "nonprod:sandbox"), or a custom endpoint.
+    
+    Returns:
+        list[str]: A list of fallback hostnames following the pattern:
+            - For nonprod: <root>.{a-e}.fallback.ably-realtime-nonprod.com
+            - For others: <endpoint>.{a-e}.fallback.ably-realtime.com
+    """

ably/types/options.py (1)

173-174: Consider adding setter for endpoint property.

While the current implementation treats endpoint as read-only, consider adding a setter to allow runtime modifications, similar to other properties in the class.

 @property
 def endpoint(self):
     return self.__endpoint
+
+@endpoint.setter
+def endpoint(self, value):
+    self.__endpoint = value
+    self.__rest_hosts = self.__get_rest_hosts()
+    self.__realtime_hosts = self.__get_realtime_hosts()

test/ably/rest/restinit_test.py (1)

79-80: Fix Yoda condition.

Rewrite the condition to improve readability.

-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

ably/types/options.py (1)

176-177: Add endpoint property documentation.

Consider adding docstring to the endpoint property to describe its purpose and valid values.

 @property
 def endpoint(self):
+    """
+    The endpoint used for host resolution. Can be:
+    - A routing policy name (e.g., "main")
+    - A non-production routing policy (e.g., "nonprod:sandbox")
+    - A fully qualified domain name (e.g., "foo.example.com")
+    """
     return self.__endpoint

test/ably/rest/restinit_test.py (3)

79-79: Fix Yoda conditions in assertions.

Update the assertions to follow Python's conventional comparison order.

-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host
-        assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host

Also applies to: 84-84

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

106-108: Update environment to endpoint in test.

For consistency with ADR-119, update the test to use endpoint instead of environment.

-        ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10)
+        ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10)
         assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted(
             ably.options.get_fallback_rest_hosts())

---

185-187: Fix Yoda conditions in URL assertions.

Update the URL assertions to follow Python's conventional comparison order.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'https://main.realtime.ably.net'

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'http://main.realtime.ably.net'

Also applies to: 194-196

🧰 Tools

🪛 Ruff (0.8.2)

185-187: Yoda condition detected

(SIM300)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/ably/rest/restinit_test.py (2)

106-112: 🛠️ Refactor suggestion

Update environment to endpoint in fallback hosts test.

For consistency with ADR-119, update the test to use endpoint instead of environment.

-        ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10)
+        ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10)
        assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted(
            ably.options.get_fallback_rest_hosts())

---

76-84: 🛠️ Refactor suggestion

Fix Yoda conditions and update environment to endpoint.

The tests use environment parameters but should be updated to use endpoint parameters for consistency with ADR-119. Also, fix the Yoda conditions flagged by the static analyzer.

        # environment: main
-        ably = AblyRest(token='foo', environment="main")
+        ably = AblyRest(token='foo', endpoint="main")
        host = ably.options.get_rest_host()
-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

        # environment: other
-        ably = AblyRest(token='foo', environment="nonprod:sandbox")
+        ably = AblyRest(token='foo', endpoint="nonprod:sandbox")
        host = ably.options.get_rest_host()
-        assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

84-84: Yoda condition detected

Rewrite as host == "sandbox.realtime.ably-nonprod.net"

(SIM300)

ably/realtime/realtime.py (1)

51-59: Documentation improvements needed.

The endpoint parameter is structured correctly, but the deprecated parameters documentation could be improved for better readability and consistency with the rest.py file.

             endpoint: str
                 Endpoint specifies either a routing policy name or fully qualified domain name to connect to Ably.
             realtime_host: str
-                Deprecated: this property is deprecated and will be removed in a future version.
-                Enables a non-default Ably host to be specified for realtime connections.
-                For development environments only. The default value is realtime.ably.io.
+                Deprecated: this property is deprecated and will be removed in a future version. Enables a non-default 
+                Ably host to be specified for realtime connections. For development environments only. 
+                The default value is realtime.ably.io.
             environment: str
-                Deprecated: this property is deprecated and will be removed in a future version.
-                Enables a custom environment to be used with the Ably service. Defaults to `production`
+                Deprecated: this property is deprecated and will be removed in a future version. Enables a custom 
+                environment to be used with the Ably service. Defaults to `production`

test/ably/rest/restinit_test.py (2)

185-187: Fix Yoda condition in HTTPS assertion.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
-            ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'https://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.8.2)

185-187: Yoda condition detected

(SIM300)

---

194-196: Fix Yoda condition in HTTP assertion.

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
-            ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'http://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.8.2)

194-196: Yoda condition detected

(SIM300)

ably/types/options.py (2)

287-290: Condition should check explicitly if rest_host is None.

Using if self.rest_host is not None: is not as clear as an explicit comparison to None. This pattern is also inconsistent with other checks in the codebase.

-            if self.rest_host is not None:
+            if self.rest_host is not None:
                 fallback_hosts = []
             else:
                 fallback_hosts = Defaults.get_fallback_hosts(self.endpoint)

---

173-174: Consider adding an endpoint.setter method.

All other properties in this class have both getter and setter methods. For consistency, it would be good to add a setter for the endpoint property as well, even if it's not currently needed.

@property
def endpoint(self):
    return self.__endpoint

+@endpoint.setter
+def endpoint(self, value):
+    self.__endpoint = value

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

test/ably/rest/restinit_test.py (3)

79-79: Fix Yoda condition

The assertion is using a Yoda condition (placing the literal first in a comparison).

-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

82-84: Properly updated environment to nonprod:sandbox with Yoda condition

The test has been correctly updated to use the new "nonprod:sandbox" environment, which aligns with the hostname changes mentioned in the PR objectives.

However, there's another Yoda condition that should be fixed:

-        assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.8.2)

84-84: Yoda condition detected

Rewrite as host == "sandbox.realtime.ably-nonprod.net"

(SIM300)

---

185-196: Fix Yoda conditions in HTTPS/HTTP tests

Both the HTTPS and HTTP tests use Yoda conditions.

For HTTPS test:

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
-            ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'https://main.realtime.ably.net'

For HTTP test:

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
-            ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'http://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.8.2)

185-187: Yoda condition detected

(SIM300)

---

194-196: Yoda condition detected

(SIM300)

ably/types/options.py (1)

46-48: Validation for endpoint vs. environment is correct but could be simplified

The validation is correct but could be simplified by using a single if statement when checking for incompatible options.

-        if environment is not None and endpoint is not None:
-            raise ValueError('specify endpoint or environment, not both')
+        if endpoint is not None and (environment is not None or rest_host is not None or realtime_host is not None):
+            raise ValueError('endpoint is incompatible with any of environment, rest_host or realtime_host')

This approach would also align with the ADR-119 requirements by checking that endpoint cannot be used with any of the legacy options (environment, restHost, or realtimeHost).

🧰 Tools

🪛 Ruff (0.8.2)

46-47: Use a single if statement instead of nested if statements

(SIM102)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

ably/transport/defaults.py (1)

47-58: ⚠️ Potential issue

FQDN handling in get_hostname needs REC2c2 implementation.

The hostname resolution logic is well-structured for different endpoint formats, but appears to be missing implementation of REC2c2 requirements for fallback behavior. This was also highlighted in previous reviews.

#!/bin/bash
# Description: Check the Ably specification for REC2c2 requirements

# Lookup ably-go implementation for comparison
rg -A 10 "REC2c2" --glob "*.py"

# Check if there's any retry/fallback logic implemented
rg -A 5 "fallback_retry_timeout" --glob "*.py"

test/ably/rest/restinit_test.py (2)

79-79: Fix Yoda condition and update hostname assertion.

The hostname assertion has been correctly updated to reflect the new main.realtime.ably.net domain, but uses a non-preferred Yoda condition format.

-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

185-187: Fix Yoda conditions in URL assertions.

The URL assertions have been correctly updated to reflect the new main.realtime.ably.net domain, but use non-preferred Yoda condition format.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'https://main.realtime.ably.net'

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'http://main.realtime.ably.net'

Also applies to: 194-196

🧰 Tools

🪛 Ruff (0.8.2)

185-187: Yoda condition detected

(SIM300)

ably/types/options.py (1)

46-48: Redundant validation check.

This validation check is redundant as it's already covered by the broader check on lines 22-24.

-        if environment is not None and endpoint is not None:
-            raise ValueError('specify endpoint or environment, not both')
-

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

test/ably/rest/restinit_test.py (3)

79-79: Fix Yoda condition.

Rewrite the condition to follow standard Python style.

-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.8.2)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

185-187: Fix Yoda condition in HTTPS URL assertion.

Rewrite condition to follow standard Python style.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'https://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.8.2)

185-187: Yoda condition detected

(SIM300)

---

194-196: Fix Yoda condition in HTTP URL assertion.

Rewrite condition to follow standard Python style.

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'http://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.8.2)

194-196: Yoda condition detected

(SIM300)

ably/types/options.py (1)

289-311: Ensure consistent fallback host handling in realtime hosts.

The code checks for self.rest_host when determining fallback hosts for REST, but doesn't do an equivalent check for realtime hosts. Consider making this behavior consistent.

    def __get_realtime_hosts(self):
        if self.realtime_host is not None:
            host = self.realtime_host
            return [host]

        host = Defaults.get_hostname(self.endpoint)
-        return [host] + self.__fallback_hosts
+        # Fallback hosts
+        fallback_hosts = self.fallback_hosts
+        if fallback_hosts is None:
+            if self.realtime_host:
+                fallback_hosts = []
+            else:
+                fallback_hosts = Defaults.get_fallback_hosts(self.endpoint)
+        
+        return [host] + fallback_hosts

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

test/ably/rest/restinit_test.py (1)

212-220: 🛠️ Refactor suggestion

Update custom environment test to use endpoint parameter

For consistency with the new endpoint approach, update the test to use the endpoint parameter and avoid using the deprecated environment parameter.

-        ably = AblyRest(token='token', environment='custom')
+        ably = AblyRest(token='token', endpoint='custom')
         with patch.object(AsyncClient, 'send', wraps=ably.http._Http__client.send) as get_mock:
             try:
                 await ably.time()
             except AblyException:
                 pass
             request = get_mock.call_args_list[0][0][0]
-            assert request.url == 'https://custom.realtime.ably.net:443/time'
+            assert request.url == 'https://custom.realtime.ably.net:443/time'

🧰 Tools

🪛 Ruff (0.11.9)

215-218: Use contextlib.suppress(AblyException) instead of try-except-pass

Replace with contextlib.suppress(AblyException)

(SIM105)

test/ably/rest/restinit_test.py (2)

82-84: 🛠️ Refactor suggestion

Use endpoint parameter instead of environment for nonprod:sandbox

Update the test to use endpoint instead of environment for consistency with ADR-119. Also fix the Yoda condition for better readability.

-        ably = AblyRest(token='foo', environment="nonprod:sandbox")
+        ably = AblyRest(token='foo', endpoint="nonprod:sandbox")
         host = ably.options.get_rest_host()
-        assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.11.9)

84-84: Yoda condition detected

Rewrite as host == "sandbox.realtime.ably-nonprod.net"

(SIM300)

---

106-108: 🛠️ Refactor suggestion

Update environment to endpoint in fallback hosts test

For consistency with ADR-119, update the test to use endpoint instead of environment.

-        ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10)
+        ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10)
         assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted(
             ably.options.get_fallback_rest_hosts())

ably/types/options.py (1)

54-58: 🛠️ Refactor suggestion

Move validation check before setting default values

The incompatibility check between endpoint and other parameters should happen before line 40, as suggested in past review comments. This allows for early returns and improves code clarity.

-        if environment is not None and endpoint is None:
-            endpoint = environment
-
-        if endpoint is None:
-            endpoint = Defaults.endpoint
+        # Move this logic earlier in the method, before line 40
+        if endpoint is None:
+            if environment is not None:
+                endpoint = environment
+            else:
+                endpoint = Defaults.endpoint

test/ably/rest/restinit_test.py (2)

185-187: Fix Yoda condition in HTTPS URL test

Improve readability by rewriting the assertion to avoid Yoda condition.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
-            ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'https://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.11.9)

185-187: Yoda condition detected

(SIM300)

---

194-196: Fix Yoda condition in HTTP URL test

Improve readability by rewriting the assertion to avoid Yoda condition.

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
-            ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        assert '{0}://{1}'.format(
+            ably.http.preferred_scheme, ably.http.preferred_host
+        ) == 'http://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.11.9)

194-196: Yoda condition detected

(SIM300)

ably/types/options.py (1)

22-25: Simplify the validation check for endpoint incompatibility

The nested if statement can be simplified to improve readability, as suggested by the static analysis.

-        if endpoint is not None:
-            if environment is not None or rest_host is not None or realtime_host is not None:
-                raise ValueError('endpoint is incompatible with any of environment, rest_host or realtime_host')
+        if endpoint is not None and (environment is not None or rest_host is not None or realtime_host is not None):
+            raise ValueError('endpoint is incompatible with any of environment, rest_host or realtime_host')

🧰 Tools

🪛 Ruff (0.11.9)

22-23: Use a single if statement instead of nested if statements

(SIM102)

test/ably/rest/restinit_test.py (1)

76-79: 🛠️ Refactor suggestion

Use endpoint parameter instead of environment for main environment

For consistency with the new endpoint-based connection approach in ADR-119, use the endpoint parameter instead of environment in the test. This better reflects the intended design changes.

-        ably = AblyRest(token='foo', environment="main")
+        ably = AblyRest(token='foo', endpoint="main")
         host = ably.options.get_rest_host()
-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        # environment: main
        ably = AblyRest(token='foo', endpoint="main")
        host = ably.options.get_rest_host()
        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.11.9)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

🤖 Prompt for AI Agents

In test/ably/rest/restinit_test.py around lines 76 to 79, replace the use of the
environment parameter with the endpoint parameter when creating the AblyRest
instance. Change the constructor call to use endpoint="main" instead of
environment="main" to align with the new endpoint-based connection approach
introduced in ADR-119.

test/ably/rest/restinit_test.py (2)

82-84: Use endpoint instead of environment and fix Yoda condition.

For consistency with ADR-119, update to use the new endpoint parameter rather than environment. Also, reverse the condition to follow standard Python comparison style.

-        ably = AblyRest(token='foo', environment="nonprod:sandbox")
+        ably = AblyRest(token='foo', endpoint="nonprod:sandbox")
         host = ably.options.get_rest_host()
-        assert "sandbox.realtime.ably-nonprod.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "sandbox.realtime.ably-nonprod.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.11.9)

84-84: Yoda condition detected

Rewrite as host == "sandbox.realtime.ably-nonprod.net"

(SIM300)

---

106-108: Update environment to endpoint in fallback hosts test.

For consistency with ADR-119, update the test to use endpoint instead of environment.

-        ably = AblyRest(token='foo', environment='nonprod:sandbox', http_max_retry_count=10)
+        ably = AblyRest(token='foo', endpoint='nonprod:sandbox', http_max_retry_count=10)
         assert sorted(Defaults.get_fallback_hosts('nonprod:sandbox')) == sorted(
             ably.options.get_fallback_rest_hosts())

test/ably/rest/restinit_test.py (6)

76-79: Use endpoint instead of environment parameter.

This test is still using the deprecated environment parameter instead of the new endpoint parameter introduced in ADR-119. While this works due to backward compatibility, it doesn't test the intended new API.

-        ably = AblyRest(token='foo', environment="main")
+        ably = AblyRest(token='foo', endpoint="main")
         host = ably.options.get_rest_host()
-        assert "main.realtime.ably.net" == host, "Unexpected host mismatch %s" % host
+        assert host == "main.realtime.ably.net", "Unexpected host mismatch %s" % host

🧰 Tools

🪛 Ruff (0.11.9)

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

112-112: Explicitly test default endpoint.

The code is calling Defaults.get_fallback_hosts() without arguments, which uses the default endpoint. For clarity, consider explicitly passing the default endpoint value from Defaults.endpoint to make the test intention more obvious.

-        assert sorted(Defaults.get_fallback_hosts()) == sorted(ably.options.get_fallback_rest_hosts())
+        assert sorted(Defaults.get_fallback_hosts(Defaults.endpoint)) == sorted(ably.options.get_fallback_rest_hosts())

---

185-187: Fix Yoda condition in URL assertion.

Rewrite the condition to follow standard Python comparison style.

-        assert 'https://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'https://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.11.9)

185-187: Yoda condition detected

(SIM300)

---

194-196: Fix Yoda condition in HTTP URL assertion.

Rewrite the condition to follow standard Python comparison style.

-        assert 'http://main.realtime.ably.net' == '{0}://{1}'.format(
+        assert '{0}://{1}'.format(
             ably.http.preferred_scheme, ably.http.preferred_host
-        )
+        ) == 'http://main.realtime.ably.net'

🧰 Tools

🪛 Ruff (0.11.9)

194-196: Yoda condition detected

(SIM300)

---

212-220: Use endpoint instead of environment for custom environment test.

This test is still using the deprecated environment parameter instead of the new endpoint parameter. Update it to test the intended new API.

-    async def test_environment(self):
-        ably = AblyRest(token='token', environment='custom')
+    async def test_endpoint(self):
+        ably = AblyRest(token='token', endpoint='custom')
         with patch.object(AsyncClient, 'send', wraps=ably.http._Http__client.send) as get_mock:
             try:
                 await ably.time()
             except AblyException:
                 pass
             request = get_mock.call_args_list[0][0][0]
             assert request.url == 'https://custom.realtime.ably.net:443/time'

🧰 Tools

🪛 Ruff (0.11.9)

215-218: Use contextlib.suppress(AblyException) instead of try-except-pass

Replace with contextlib.suppress(AblyException)

(SIM105)

---

14-224: Add explicit tests for the new endpoint functionality.

The test suite is missing explicit tests for several key features of the endpoint implementation as specified in ADR-119:

1. FQDN endpoints (e.g., "foo.example.com")
2. Validation of incompatible option combinations
3. Default fallback behavior for different endpoint types

Consider adding the following test cases:

@dont_vary_protocol
def test_endpoint_fqdn():
    # Test FQDN as endpoint
    ably = AblyRest(token='foo', endpoint="foo.example.com")
    host = ably.options.get_rest_host()
    assert host == "foo.example.com", "Unexpected host mismatch %s" % host
    # No fallback hosts for FQDN endpoints
    assert ably.options.get_fallback_rest_hosts() == []

@dont_vary_protocol
def test_endpoint_incompatible_options():
    # Test endpoint with environment
    with pytest.raises(ValueError, match="endpoint is incompatible"):
        AblyRest(token='foo', endpoint="main", environment="sandbox")
    
    # Test endpoint with rest_host
    with pytest.raises(ValueError, match="endpoint is incompatible"):
        AblyRest(token='foo', endpoint="main", rest_host="some.other.host")
    
    # Test endpoint with realtime_host
    with pytest.raises(ValueError, match="endpoint is incompatible"):
        AblyRest(token='foo', endpoint="main", realtime_host="some.other.host")

🧰 Tools

🪛 Ruff (0.11.9)

74-74: Yoda condition detected

Rewrite as ably.options.rest_host == "some.other.host"

(SIM300)

---

79-79: Yoda condition detected

Rewrite as host == "main.realtime.ably.net"

(SIM300)

---

84-84: Yoda condition detected

Rewrite as host == "sandbox.realtime.ably-nonprod.net"

(SIM300)

---

116-116: Yoda condition detected

Rewrite as ably.options.fallback_retry_timeout == 600000

(SIM300)

---

118-118: Yoda condition detected

Rewrite as ably.options.fallback_retry_timeout == 1000

(SIM300)

---

123-123: Yoda condition detected

Rewrite as ably.options.realtime_host == "some.other.host"

(SIM300)

---

128-128: Yoda condition detected

Rewrite as Defaults.get_port(ably.options) == 9999

(SIM300)

---

134-134: Yoda condition detected

Rewrite as Defaults.get_port(ably.options) == 9998

(SIM300)

---

140-140: Yoda condition detected

Rewrite as Defaults.get_port(ably.options) == 9999

(SIM300)

---

185-187: Yoda condition detected

(SIM300)

---

194-196: Yoda condition detected

(SIM300)

---

207-207: Yoda condition detected

Rewrite as excinfo.value.status_code == 401

(SIM300)

---

208-208: Yoda condition detected

Rewrite as excinfo.value.code == 40103

(SIM300)

---

209-209: Yoda condition detected

(SIM300)

---

215-218: Use contextlib.suppress(AblyException) instead of try-except-pass

Replace with contextlib.suppress(AblyException)

(SIM105)

ably/types/options.py (4)

54-58: Consider moving the endpoint initialization closer to the validation logic.

For better code organization and readability, consider moving these lines up to follow immediately after the incompatible options check. This will group all endpoint-related logic together.

# Move these lines to follow line 25
+        if environment is not None and endpoint is None:
+            endpoint = environment
+
+        if endpoint is None:
+            endpoint = Defaults.endpoint

---

174-176: Add setter for endpoint property.

Most properties in this class have setters, but the endpoint property does not. For consistency and to support runtime changes to the endpoint, consider adding a setter.

    @property
    def endpoint(self):
        return self.__endpoint

+    @endpoint.setter
+    def endpoint(self, value):
+        self.__endpoint = value
+        # Update hosts when endpoint changes
+        self.__rest_hosts = self.__get_rest_hosts()
+        self.__realtime_hosts = self.__get_realtime_hosts()

---

288-291: Simplify the conditional logic for fallback hosts.

The current condition checks if rest_host is not None, but the variable name doesn't match the property name used elsewhere (self.rest_host). Make sure the condition uses the correct property access for consistency.

-            if self.rest_host is not None:
+            if self.rest_host is not None:
                 fallback_hosts = []
-            else:
+            else:
                 fallback_hosts = Defaults.get_fallback_hosts(self.endpoint)

---

303-309: Consider reusing get_rest_hosts logic for get_realtime_hosts.

The __get_realtime_hosts method duplicates some of the logic from __get_rest_hosts. Consider refactoring to eliminate this duplication.

    def __get_realtime_hosts(self):
        if self.realtime_host is not None:
            host = self.realtime_host
            return [host]

-        host = Defaults.get_hostname(self.endpoint)
-        return [host] + self.__fallback_hosts
+        # Reuse the same hostname logic as rest hosts
+        if self.rest_host is not None:
+            # If rest_host is specified but realtime_host isn't, don't use fallbacks
+            # This matches the behavior for rest hosts
+            return [Defaults.get_hostname(self.endpoint)]
+        else:
+            # Use the same host logic and fallbacks as rest
+            host = Defaults.get_hostname(self.endpoint)
+            return [host] + self.__fallback_hosts

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

ably/types/options.py (1)

72-76: Backward compatibility mapping from environment to endpoint is correct.

The logic properly supports legacy environment parameter while transitioning to endpoint. Consider adding deprecation warnings for environment, rest_host, and realtime_host as mentioned in past reviews to guide users toward the new API.

ably/transport/defaults.py (1)

61-62: Consider using consistent string slicing for nonprod prefix removal.

Line 52 uses endpoint[len("nonprod:"):] while line 62 uses endpoint.replace("nonprod:", ""). While both work correctly given the startswith guard, using the same approach would improve consistency.

♻️ Suggested change

         if endpoint.startswith("nonprod:"):
-            root = endpoint.replace("nonprod:", "")
+            root = endpoint[len("nonprod:"):]
             return [

test/ably/testapp.py (1)

20-20: Variable naming mismatch between env var and usage.

The environment variable is now ABLY_ENDPOINT (line 20), but the local variable is still named environment and passed as environment=environment (line 33). While this works due to backward compatibility in Options, the naming is misleading.

Consider renaming to clarify intent:

♻️ Suggested rename for clarity

-environment = os.environ.get('ABLY_ENDPOINT', 'nonprod:sandbox')
+endpoint = os.environ.get('ABLY_ENDPOINT', 'nonprod:sandbox')

And update line 33 accordingly:

 ably = AblyRest(token='not_a_real_token',
                 port=port, tls_port=tls_port, tls=tls,
-                environment=environment,
+                endpoint=endpoint,
                 use_binary_protocol=False)

Similar updates would be needed in get_options (line 91) and test_vars (line 56).

Also applies to: 33-33

test/ably/testapp.py (1)

92-102: Unused Options object in clear_test_vars.

The Options object created on line 95 and configured on lines 96-98 is never used. The ably client is created via get_ably_rest() on line 99, which constructs its own options internally. This appears to be pre-existing dead code, but worth cleaning up.

♻️ Suggested cleanup

     `@staticmethod`
     async def clear_test_vars():
         test_vars = TestApp.__test_vars
-        options = Options(key=test_vars["keys"][0]["key_str"])
-        options.port = test_vars["port"]
-        options.tls_port = test_vars["tls_port"]
-        options.tls = test_vars["tls"]
         ably = await TestApp.get_ably_rest()
         await ably.http.delete('/apps/' + test_vars['app_id'])
         TestApp.__test_vars = None
         await ably.close()

ably/types/options.py (2)

73-85: Deprecation uses logging instead of warnings.warn.

The implementation uses log.warning() for deprecation notices rather than the standard warnings.warn() with DeprecationWarning. While this works, using warnings.warn() would be more idiomatic for deprecations as it integrates with Python's warning filters and test frameworks that capture deprecation warnings.

This is a minor consideration - the current approach is functional and provides visibility to users.

---

323-324: Consider guarding against empty hosts list.

If http_max_retry_count is set to 0, __hosts will be empty and get_host() will raise an IndexError. Consider adding a guard or clearer error message.

🛡️ Suggested guard

 def get_host(self):
+    if not self.__hosts:
+        raise ValueError("No hosts available; check http_max_retry_count and endpoint configuration")
     return self.__hosts[0]